fix(web): chat pane preserves scroll position when todo card grows#2299
fix(web): chat pane preserves scroll position when todo card grows#2299neogenix wants to merge 11 commits into
Conversation
|
|
||
| const WEB_ROOT = dirname(fileURLToPath(import.meta.url)); | ||
| const WORKSPACE_ROOT = dirname(dirname(WEB_ROOT)); | ||
| const WORKSPACE_ROOT = process.env.OD_WORKSPACE_ROOT ?? dirname(dirname(WEB_ROOT)); |
There was a problem hiding this comment.
OD_WORKSPACE_ROOT is used verbatim here, unlike the other path overrides in this file. That means an empty string or a relative path will flow straight into outputFileTracingRoot and turbopack.root, which makes the new worktree override fail in opaque ways instead of either resolving predictably or failing fast. Please normalize/validate this env var before using it here (for example: ignore blank values, and either resolve relative paths against WEB_ROOT or throw unless the override is absolute).
| // If the viewport is shorter than 150 px of content the scroll moved us away; | ||
| // if the chat-log has no scrollable room (few messages, large viewport) the | ||
| // scroll will be a no-op and the test is a guaranteed pass. Both are fine. | ||
| const scrollUpOccurred = distanceAfterScroll > 80; |
There was a problem hiding this comment.
This branch lets scenario B pass without asserting anything if the synthetic scroll-up does not move the log more than 80px from the bottom. That creates a false-green path where a later layout or seed change stops exercising the regression entirely, but the test still passes. Please make the precondition explicit instead: assert scrollable overflow and distanceAfterScroll > 80 before growPinnedTodo, so the spec fails fast when it no longer reaches the non-pinned path it is supposed to verify.
|
|
||
| function resolveWorkspaceRoot(): string { | ||
| const override = process.env.OD_WORKSPACE_ROOT; | ||
| if (override && override.trim()) { |
There was a problem hiding this comment.
OD_WORKSPACE_ROOT is still treated as valid as soon as it is non-blank, even though this branch introduced it specifically as a config override. If the value points at a missing directory or a directory that does not actually contain apps/web, Next will fail later inside file tracing / Turbopack with a much harder-to-diagnose error instead of failing at config load time. The evidence is in resolveWorkspaceRoot(): the override is trimmed and resolved, but never checked for existence or containment before being assigned to outputFileTracingRoot and turbopack.root. Please validate the resolved path here (for example, stat/realpath it and throw when it does not exist or when relative(resolvedRoot, WEB_ROOT) escapes the root) so this new override follows the repo's fail-fast rule.
| // scroll will be a no-op and the test is a guaranteed pass. Both are fine. | ||
| const scrollUpOccurred = distanceAfterScroll > 80; | ||
|
|
||
| if (scrollUpOccurred) { |
There was a problem hiding this comment.
Scenario B can still go false-green because the actual regression assertion only runs inside if (scrollUpOccurred). If a later layout, viewport, or seed change leaves distanceAfterScroll <= 80, this test now exits cleanly without ever exercising the "user scrolled up, do not yank them back" path it claims to cover. The new precondition check helps only after the branch is entered, so it does not close the original gap. Please make the precondition mandatory instead: assert scrollable overflow first, then fail if the manual scroll does not move the log beyond the 80px threshold (or keep scrolling until it does) before calling growPinnedTodo().
| if (!existsSync(resolved)) { | ||
| console.warn( | ||
| `[next.config] OD_WORKSPACE_ROOT="${override}" resolved to "${resolved}" which does not exist; ` + | ||
| `falling back to computed default "${computed}".`, | ||
| ); | ||
| return computed; | ||
| } | ||
| const rel = relative(resolved, WEB_ROOT); | ||
| if (rel.startsWith('..')) { | ||
| console.warn( | ||
| `[next.config] OD_WORKSPACE_ROOT="${override}" resolved to "${resolved}" but WEB_ROOT "${WEB_ROOT}" ` + | ||
| `escapes it (relative path "${rel}"); falling back to computed default "${computed}".`, | ||
| ); | ||
| return computed; | ||
| } |
There was a problem hiding this comment.
resolveWorkspaceRoot() now treats an invalid OD_WORKSPACE_ROOT as a warning-and-fallback path instead of failing at config load. That matters because this PR introduced the override specifically to make worktree Playwright runs deterministic; a typo or stale path can silently switch outputFileTracingRoot/turbopack.root back to the computed default and send the run back to the original opaque resolution behavior. The evidence is in these branches: both the missing-directory case and the WEB_ROOT-outside-root case call console.warn(...) and return computed. Please replace those fallbacks with a thrown configuration error (or equivalent hard failure) so an explicitly provided but invalid OD_WORKSPACE_ROOT fails fast with a clear message.
| const rel = relative(resolved, WEB_ROOT); | ||
| if (rel.startsWith('..')) { | ||
| throw new Error( | ||
| `OD_WORKSPACE_ROOT="${override}" resolved to "${resolved}" but WEB_ROOT "${WEB_ROOT}" ` + | ||
| `is not inside it (relative path "${rel}"). ` + | ||
| `The override must be an ancestor of apps/web.`, | ||
| ); | ||
| } | ||
| return resolved; |
There was a problem hiding this comment.
This validation still accepts values that are ancestors of apps/web but are not actually valid workspace roots for the web app. For example, OD_WORKSPACE_ROOT=<repo>/apps or <repo>/apps/web passes this relative(resolved, WEB_ROOT) check, but WORKSPACE_ROOT is then wired into outputFileTracingRoot and turbopack.root below while the app imports workspace packages from packages/* (for example @open-design/contracts in apps/web/src/components/ChatPane.tsx). That means the new override can still bless a broken path and send the worktree/dev run back to incomplete tracing or opaque module-resolution failures. Please validate the override as the actual repository workspace root instead of any ancestor of apps/web—for example require pnpm-workspace.yaml and/or the expected top-level workspace directories at resolved.
| const distanceAfterGrow = await chatLogBottomDistance(page); | ||
| // The scroll position should be roughly preserved (within 20 px tolerance | ||
| // for rounding or minor layout shifts the browser may apply). | ||
| expect( | ||
| distanceAfterGrow, | ||
| `expected scroll position preserved after todo card grew (before=${distanceAfterScroll} after=${distanceAfterGrow})`, | ||
| ).toBeGreaterThan(60); |
There was a problem hiding this comment.
This assertion is weaker than the contract the comment describes. The test says scenario B should preserve the user's scroll position within about 20px, but toBeGreaterThan(60) still passes if a regression drags the log most of the way back to the bottom—for example distanceAfterScroll = 150 and distanceAfterGrow = 61. That can false-green the new regression coverage even though the user's position changed materially. Please compare distanceAfterGrow against distanceAfterScroll directly (for example Math.abs(distanceAfterGrow - distanceAfterScroll) <= 20) instead of only checking that it stays somewhere above 60.
|
Fixed in 7a1b1c8 — addresses the three remaining @nettee findings:
|
| `Fix the path or unset the variable to use the computed default.`, | ||
| ); | ||
| } | ||
| const rel = relative(resolved, WEB_ROOT); |
There was a problem hiding this comment.
OD_WORKSPACE_ROOT was added here for worktree / symlinked checkouts, but this ancestor check is still purely lexical. On macOS /tmp vs /private/tmp (and the same kind of aliasing through symlinked worktree paths), both strings can name the same physical repo while path.relative() still returns a .. path, so this branch throws even though the override is a valid physical ancestor. The evidence is in the changed lines here: resolved is validated with relative(resolved, WEB_ROOT) and then used for the pnpm-workspace.yaml lookup without canonicalizing either side first. That means the new escape hatch can still reject exactly the non-standard checkout layouts it was introduced to support. Please canonicalize both resolved and WEB_ROOT with realpathSync before the ancestor and workspace-root checks, then validate the canonical paths instead.
The PinnedTodoSlot renders outside the chat-log scroll container. When the todo card grows (new tasks added via TodoWrite), the scroll container's clientHeight shrinks in the flex layout, drifting the user away from the bottom. The existing ResizeObserver only observed children of the chat-log div, so pinned-todo growth was invisible to followLatestIfPinned. Fix: pass a containerRef to PinnedTodoSlot and observe that element in the same ResizeObserver. syncPinnedTodo() is called on effect setup and from the MutationObserver callback so observation stays current as the slot appears and disappears across TodoWrite snapshots. Red spec: apps/web/tests/components/chat-todo-autoscroll.test.tsx
…rows Clarify test comment: the second test confirms followLatestIfPinned snaps scroll to bottom when fired. The structural guarantee (pinned-todo element is observed) is separately asserted in test 1, which is the check that goes red on main without the fix.
…nnedTodoSlot mount detection The MutationObserver was only watching the .chat-log element. PinnedTodoSlot (.chat-pinned-todo) is a sibling of .chat-log-wrap inside .pane, outside the observed subtree. syncPinnedTodo inside the MutationObserver callback was therefore dead code for mount/unmount transitions of the slot. Add a second observation on paneEl (el.parentElement?.parentElement) with childList-only so the MutationObserver fires when PinnedTodoSlot mounts or unmounts and syncPinnedTodo can register/deregister the element with the ResizeObserver.
Add Playwright spec that goes red on origin/main and green on this fix branch. Scenario A asserts that a chat-log pinned to the bottom snaps back after the PinnedTodoCard grows (the ResizeObserver-on-pinned-todo path). Scenario B asserts that a deliberate scroll-up is not overridden. Also allow OD_WORKSPACE_ROOT env override in next.config.ts so Turbopack resolves node_modules correctly when the web app is booted from a worktree whose node_modules symlinks resolve outside the default workspace root.
PinnedTodoSlot sits outside the .chat-log scroll container, so the ResizeObserver and MutationObserver coverage that keeps auto-scroll working when the todo card grows is non-obvious to future implementers. Document the invariant in the Chat UI conventions section.
…ndition unconditional
… test branch Three follow-ups to nettee's review feedback: 1. apps/web/next.config.ts gains a pnpm-workspace.yaml existence check after the relative-path validation. Without it, an override like '<repo>/apps' or '<repo>/apps/web' passes the relative(resolved, WEB_ROOT) check but the resolved path is missing the sibling packages/* directory that apps/web imports from (for example @open-design/contracts). Next would later fail deep inside file tracing / Turbopack with a much harder-to-diagnose error. Now we throw at config load with a clear message. 2. e2e/ui/chat-todo-autoscroll.test.ts drops the redundant 'if (scrollUpOccurred)' branch. The hard precondition above it already guarantees distanceAfterScroll > 80, so the if was dead code that read as a false-green path. The body now runs unconditionally. 3. Same test tightens the post-grow assertion. The previous toBeGreaterThan(60) would pass even if a regression dragged the log most of the way back to the bottom (e.g. before=150, after=61). Replaced with Math.abs(distanceAfterGrow - distanceAfterScroll) less than SCROLL_PRESERVATION_TOLERANCE_PX (20) — a delta check that actually verifies the comment's claim of 'within ~20px of where the user left it'.
7a1b1c8 to
ddad708
Compare
| const rel = relative(resolved, WEB_ROOT); | ||
| if (rel.startsWith('..')) { | ||
| throw new Error( | ||
| `OD_WORKSPACE_ROOT="${override}" resolved to "${resolved}" but WEB_ROOT "${WEB_ROOT}" ` + | ||
| `is not inside it (relative path "${rel}"). ` + | ||
| `The override must be an ancestor of apps/web.`, | ||
| ); |
There was a problem hiding this comment.
relative(resolved, WEB_ROOT).startsWith('..') is still not a sufficient ancestor check here. On Windows, path.relative() returns an absolute drive-qualified path when the two inputs are on different drives, so an override like D:\repo against C:\repo\apps\web yields C:\repo\apps\web instead of a ..-prefixed path. That means this new fail-fast validation can still bless an unrelated workspace root whenever the other drive also contains pnpm-workspace.yaml, and Next then traces the wrong tree instead of failing early. Please reject absolute rel values as well (for example rel === '' || (!rel.startsWith('..') && !isAbsolute(rel))) or replace this branch with a real ancestor predicate before the workspace-root check.
There was a problem hiding this comment.
Fixed in 2c04814. Both concerns addressed:
realpathSyncis now applied to bothresolvedandWEB_ROOTbefore therelative()call, so symlinked paths (macOS/tmp→/private/tmp, worktree checkouts via symlinked dirs) compare against the same canonical filesystem paths.isAbsolute(rel)guard added alongsiderel.startsWith(".."), catching the Windows cross-drive case wherepath.relative()returns an absolute path instead of a..-prefixed string.
| // Now grow the todo card — the non-pinned user should NOT be dragged back. | ||
| await growPinnedTodo(page, 80); | ||
|
|
||
| const distanceAfterGrow = await chatLogBottomDistance(page); | ||
|
|
||
| // Tight invariant: the scroll position should be preserved within a small | ||
| // delta of where the user left it. A regression that pulls the log most of | ||
| // the way back to the bottom (e.g. before=150, after=61) would have passed | ||
| // a loose absolute threshold but fails this delta check. | ||
| const SCROLL_PRESERVATION_TOLERANCE_PX = 20; | ||
| expect( | ||
| Math.abs(distanceAfterGrow - distanceAfterScroll), | ||
| `expected scroll position preserved within ${SCROLL_PRESERVATION_TOLERANCE_PX}px of pre-grow ` + | ||
| `(before=${distanceAfterScroll} after=${distanceAfterGrow} delta=${Math.abs(distanceAfterGrow - distanceAfterScroll)})`, | ||
| ).toBeLessThan(SCROLL_PRESERVATION_TOLERANCE_PX); |
There was a problem hiding this comment.
Scenario B is asserting the wrong invariant for the non-pinned branch. growPinnedTodo() above intentionally shrinks .chat-log.clientHeight and reapplies the pre-grow scrollTop; when the user has scrolled up and followLatestIfPinned stays suppressed, the correct behavior is to preserve that scrollTop/visible content, which naturally makes scrollHeight - scrollTop - clientHeight grow by roughly the amount the viewport shrank. Comparing distanceAfterGrow to distanceAfterScroll therefore fails on correct behavior, or only passes when the grow step never actually changed layout. Please assert a preserved scrollTop (or a stable visible message anchor) instead, and add a hard precondition that the grow step really changed clientHeight so this regression coverage cannot false-green.
There was a problem hiding this comment.
Fixed in 2c04814. Scenario B now:
- Captures
scrollTopandclientHeightbeforegrowPinnedTodo. - Hard precondition: asserts
clientHeightdecreased after the grow (fails fast if layout stops exercising this path). - Core assertion:
Math.abs(scrollTopAfterGrow - scrollTopBeforeGrow) < 20— directly tests that the user's scroll position was not yanked. The old distance-to-bottom delta was wrong becausegrowPinnedTodoreducesclientHeightbyextraPxwhile holdingscrollTopfixed, sodistanceAfterGrow ≈ distanceAfterScroll + 80is correct behavior, not a preserved position.
…cenario B assertion - Use realpathSync on both resolved and WEB_ROOT before the ancestor check so that symlinked paths (macOS /tmp vs /private/tmp, worktree checkouts) compare correctly instead of false-throwing on a physically valid override. - Add isAbsolute(rel) guard for the Windows cross-drive case where path.relative() returns an absolute path instead of a ..-prefixed string. - Scenario B: replace distance-to-bottom delta assertion with scrollTop preservation check. Growing the pinned todo naturally increases distance-to-bottom by ~extraPx (clientHeight shrinks while scrollTop is held fixed), so the old Math.abs(after - before) < 20 check would fail on correct behavior. asserting scrollTop directly catches the real regression: followLatestIfPinned incorrectly snapping a non-pinned user back to the bottom. - Add hard precondition that clientHeight actually changed so the test fails fast if the layout stops exercising the non-pinned path.
|
|
||
| // Grow the pinned-todo card by 80 px (simulates a new TodoWrite snapshot with | ||
| // more items) and verify the chat-log snaps back to the bottom. | ||
| await growPinnedTodo(page, 80); |
There was a problem hiding this comment.
Scenario A still never proves that growPinnedTodo() actually changed the layout it is supposed to exercise. The only hard precondition here is scrollable overflow, so if a later CSS/layout change stops .chat-pinned-todo from shrinking .chat-log.clientHeight, distanceAfterGrow < 20 will still pass because the log was already pinned before the grow. That would false-green the main regression this spec is meant to protect. Please capture .chat-log.clientHeight before and after growPinnedTodo(page, 80) in scenario A and assert it decreases before checking distanceAfterGrow, the same way scenario B now fails fast when the grow step stops exercising the non-pinned path.
| // When the todo card grows, the chat-log viewport (clientHeight) | ||
| // shrinks. The ResizeObserver must observe the pinned-todo div so | ||
| // `followLatestIfPinned` fires and corrects the scroll position. | ||
| render(chatPaneEl(messagesWithTodo(3))); |
There was a problem hiding this comment.
The new ChatPane behavior here depends on the pane-level MutationObserver re-running syncPinnedTodo() when PinnedTodoSlot mounts or remounts, but both tests in this file start with a TodoWrite card already present and then only exercise resize on an already-observed element. If a refactor breaks the new mount/unmount wiring while leaving resize-on-existing-card intact, this suite still passes and misses the regression introduced in the changed ChatPane lines. Please add a rerender case that starts without a TodoWrite snapshot, then mounts one (and optionally remounts after dismiss) and asserts the new .chat-pinned-todo element becomes observed / still triggers followLatestIfPinned.
nettee
left a comment
There was a problem hiding this comment.
@neogenix I reviewed the current head's ChatPane observer wiring, the OD_WORKSPACE_ROOT guard, and the new jsdom/Playwright regression coverage. The mount/unmount observer path and both pinned/non-pinned scroll invariants now line up with the implementation, and I don't see any remaining actionable issues in the changed ranges. Thanks for tightening both the behavior fix and the regression coverage here.
Why
While running a complex session with the agent making frequent TodoWrite
updates, I noticed the chat pane stopped following the latest output
whenever a new task was added to the pinned TodoCard. The chat log
silently drifted off-bottom even though the user had not scrolled.
Tracing it:
PinnedTodoSlotrenders OUTSIDE the.chat-logscrollcontainer — it sits as a sibling of
.chat-log-wrapinside.pane, NOTas a child. The existing
ResizeObserverinChatPane.tsxonly observedchildren of
.chat-log, so when the todo card grew, the chat log'sclientHeightshrank in the flex layout and the user drifted away fromthe bottom without
followLatestIfPinnedever firing.The existing scroll-pin logic (
pinnedToBottomRef, the 80 px cutoff,the "preserve position if user scrolled up" branch) was already correct.
The ResizeObserver just wasn't seeing the source of the resize.
What users will see
When TodoWrite emits a new snapshot that adds items to the pinned task
card, the chat log auto-scrolls to keep the latest output visible — but
only when the user is already pinned near the bottom (within 80 px). If
the user has scrolled up to read earlier output, their position is
preserved exactly as before. This is the same smart-follow behavior
Discord and terminal apps use; no new setting needed.
Surface area
No new CLI subcommand needed: this is a pure UI behavior change, not a
new capability. The chat pane already exists; only its auto-scroll
trigger surface was incomplete.
Screenshots
The fix is a behavior change visible only in motion. Reproduction: start
any long agent session that calls TodoWrite multiple times, watch the
chat log, observe that on
mainthe scroll drifts up as the todo cardgrows; on this branch the chat log stays pinned to bottom unless the
user has deliberately scrolled up.
Bug fix verification
apps/web/tests/components/chat-todo-autoscroll.test.tsxobservedElementscontains the.chat-pinned-tododiv.main: structural failure (the new ref path doesn't exist).e2e/ui/chat-todo-autoscroll.test.tsRed on
mainwithdistance = 995 px, green here withdistance = 0.subsequent TodoCard growth.
Validation
pnpm guard(clean)pnpm typecheck(clean)pnpm --filter @open-design/web test— 9/9 chat-suite tests pass.32 unrelated pre-existing failures in
useCritiqueTheaterEnabled,AssistantMessage,DesignsTab.select-mode,SettingsDialog.execution,WorkspaceTabsBarare present onmainand unchanged here.pnpm exec playwright test -c playwright.config.ts chat-todo-autoscroll— 2 scenarios pass (browser-verified in Chromium).
Adjacent issues (out of scope)
ChatPane.tsx(lines ~423 and~466) ignore
prefers-reduced-motion. Not introduced here; deferredas a separate a11y pass.
apps/web/next.config.tsgains a smallOD_WORKSPACE_ROOTenv-varfallback so Turbopack can resolve
apps/web/node_moduleswhen thePlaywright harness runs from a worktree whose
node_modulesis asymlink pointing outside the worktree root. Default behavior is
unchanged (the override only fires when the env var is explicitly
set). CI runs from the main checkout where the default already
resolves correctly. Included here only because the
e2e/ui/Playwright spec requires it during local-worktree dev.
AGENTS.md"Chat UI conventions" gains one sentence documenting thePinnedTodoSlot observer coverage so future implementers don't trip
over the same out-of-subtree gotcha.